-
Notifications
You must be signed in to change notification settings - Fork 88
feat: Support foreign accounts #1486
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ransactionInputsDataStore
| } | ||
|
|
||
| let foreign_inputs = | ||
| self.tx_inputs.read_foreign_account_inputs(foreign_account_id).unwrap(); // todo unwrap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bobbinth do we need the impl for TransactionInputsDataStore, rather than just NtxDataStore?
If yes, do you imagine the impl looking like this - in the sense that account inputs are derived from tx inputs. The read_foreign_account_inputs fn is a stub for now in a branch of mine, just trying to flesh out integration first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here we should be able to get all the data from the TransactionInputs - so, it would work differently from NtxDataStore. Basically, the main difference between the validator and NTX builder is:
- NTX builder: executes a transaction from scratch and so it doesn't now what execution path will be taken during transaction execution. For example, it doesn't know which foreign accounts would need to be loaded ahead of time, and so, we need to load them dynamically (i.e., by fetching the requested accounts from the store).
- Validator: re-executes transaction that was previously executed by the user/NTX builder. All foreign account data should already be loaded into the
TransactionInputs. The main challenge here would be to figure out how to extract the relevant data fromTransactionInputsas it is not too structured. I believe I left a comment somewhere sketching out how to extract this data (and maybe putting some logic for this inmiden-basewould simplify things).
a7edd4f to
9df9847
Compare
| } | ||
| // TODO(currentpr): We don't have tx inputs here so what do we do? | ||
| let foreign_inputs = | ||
| self.tx_inputs.read_foreign_account_inputs(foreign_account_id).unwrap(); // todo unwrap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bobbinth assuming we expect to get foreign account inputs from tx inputs, is it possible to do so here? Should we somehow retrieve the tx inputs through the store or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we can get foreign account inputs from the original TX inputs - or at least it may not always work.
I think to get foreign account inputs, we'd need to make a request to the store and load the account from there. We may also want to cache the foreign account NtxDataStore so that if there are additional requests, we won't need to go to the store again for the same data.
There is one nuance here though: currently, we are able to fetch the full account from the store, but this functionality will go away after the refactoring @drahnr is working on. So, it may make sense to add a dedicated endpoint to the data store that would return the data sufficient to construct AccountInputs (i.e., the PartialAccount and AccountWitness structs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any advice on how to do this? Seems like we would have to get the full account from store state, convert that to partial account + get account witness somehow.
it may make sense to add a dedicated endpoint to the data store that would return the data sufficient to construct AccountInputs (i.e., the PartialAccount and AccountWitness structs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we have the ability to get the full account from the store, things should be pretty easy:
- The get the partial account, we get the full account and then convert it to partial account (we have
impl From<&Account> for PartialAccountimplemented). - The witness we'd get from the
AccountTree- that should be pretty straight-forward too.
I think both of these we should be able to get from the current GetAccountProof endpoint. But soon, the ability to get full account will go away.
After a series of @drahnr's PRs gets merged, GetAccountProof becomes GetAccount and it no longer would return the full account. But it would still return enough info to build a PartialAccount + account witness. For partial account, all we really need is account_id, vault root, storage header, code, and nonce - and I believe all of this should be available from the GetAccount endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a series of @drahnr's PRs gets merged, GetAccountProof becomes GetAccount and it no longer would return the full account. But it would still return enough info to build a PartialAccount + account witness. For partial account, all we really need is account_id, vault root, storage header, code, and nonce - and I believe all of this should be available from the GetAccount endpoint.
All of this will be available once #1385 is merged as GetAccount
cd2b667 to
ae1b914
Compare
| let account_proof = store.get_account(account_id, None).await.map_err(|err| { | ||
| DataStoreError::Other { | ||
| error_msg: "failed to open vault asset tree".into(), | ||
| error_msg: format!("Failed to get account inputs from store: {err}").into(), | ||
| source: Some(Box::new(err)), | ||
| } | ||
| }) | ||
| })) | ||
| })?; | ||
|
|
||
| // Construct vault from account details. | ||
| let account_details = | ||
| account_proof.details.ok_or_else(|| DataStoreError::Other { | ||
| error_msg: "account proof does not contain account details".into(), | ||
| source: None, | ||
| })?; | ||
| let asset_vault = | ||
| AssetVault::new(&account_details.vault_details.assets).map_err(|err| { | ||
| DataStoreError::Other { | ||
| error_msg: format!("failed to create asset vault: {err}").into(), | ||
| source: Some(Box::new(err)), | ||
| } | ||
| })?; | ||
|
|
||
| get_asset_witnesses(vault_keys, &asset_vault) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments on this approach:
First, when we make a request to the store, we should use the reference block of this data store (i.e., self.reference_header.block_num().
But also, getting the full account proof is very heavy compared to what we need here. Maybe that's OK for this PR, but we should follow it up with using dedicated endpoints on the store to get asset/storage witnesses. Let's create an issue for this.
| let foreign_inputs = self | ||
| .tx_inputs | ||
| .read_foreign_account_inputs(account_id) | ||
| .map_err(|err| DataStoreError::Other { | ||
| error_msg: format!("failed to read foreign account inputs: {err}").into(), | ||
| source: Some(Box::new(err)), | ||
| })?; | ||
| get_asset_witnesses_from_account(foreign_inputs.account(), vault_keys) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if extracting account inputs first and then trying to get asset witness from the account inputs will work (though, it depends on how read_foreign_account_inputs() works).
An alternative approach would be to read the asset witness directly. It should actually be much simpler than reading the whole account inputs object (we should be able to look up the path based on the vault_root in the Merkle store).
| let foreign_inputs = self | ||
| .tx_inputs | ||
| .read_foreign_account_inputs(account_id) | ||
| .map_err(|_| DataStoreError::AccountNotFound(account_id))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment as above, but about reading storage witnesses directly rather than going though account inputs.
|
Moving the Validator impl to this PR #1493 |
|
Closing in favour of #1521 |
Context
Neither the Validator's
TransactionInputsDataStorenor the NTX Builder'sNtxDataStorehandle foreign accounts for the purposes of the following functions ofDataStoretrait:get_foreign_account_inputs()get_foreign_account_inputs()get_vault_asset_witnesses()NOTE: This will remain in draft until miden-base has the
TransactionIniputs::read_foreign_account_inputs()functionality merged into next.Closes #1387 and #1305.
Changes
TransactionInputsDataStoreto handle foreign accounts throughself.tx_inputs.NtxDataStoreto handle foreign accounts by accessingAccountProofResponsefrom the store.